Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature(gatsby): Extract non-css-in-js css and add add to <head> when SSRing in dev #28471
feature(gatsby): Extract non-css-in-js css and add add to <head> when SSRing in dev #28471
Changes from all commits
ee315ed
b94ced1
0228936
135e960
b96c520
fee4f5a
36e7f9e
3357d79
b3bb3b4
64bc7f9
8e5f6f0
468a286
410424c
d27291f
68b32ad
e6b4c39
9ecb314
84a4f2c
3e8fa67
baf7a65
31354f2
92e4656
5a35154
d4ae483
d4d5340
59148ae
08ee647
c1eeff7
1f9fbdc
1125d6c
51bbd3b
4fbb812
e563bda
d59c6e2
627239c
1afed99
3c088a5
139efda
6d3a64b
b1506c0
9c34498
0b78528
a22361f
f01a5fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you do
_.get(stats, `namedChunkGroups`)
overstats.namedChunkGroups
? I don't understand this. Even for_.get(stats, fetchKey)
, I thinkstats[fetchKey]
is better.If the object may not exist, we should use
stats?.fetchKey
. For a default we should usestats?.fetchKey ?? defValue
. Either way, I think there are very few cases where I could see_.get
be preferred at this point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copy/pasted this from static-entry.js which was written a long long time ago pre fancy-dancy stuff like
?
. Is there a reason to refactor other than preference? I'd rather keep the implementation of the two the same.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason is technical debt. I don't think this is a preference but rather about redundant outdated code.
My suggestion; if there's a strong desire to keep them both, is to share this code somehow, rather than to copy/paste. It may be copy/paste now, but in some future somebody will look at this file and not realize that this is the case, just like I didn't know when I was reviewing this.
If nothing else, and there's a desire to keep them in sync, there ought to be a comment or notice about this somewhere in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracting out to a common function is a great idea — I hadn't done that earlier as things were in flux but let's do that & then we can refactor/modernize the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this separately, extracting to common util can have some impact on regular ssr, and we want to backport this for tomorrow's release so it's safer to ship this as-is (as everything here touches pretty much things behind the flag) and we can make common util (and also address outdated code / unneeded use of lodash too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the whole point of
flatten
to not need this? :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be simplified to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.sort
expects0
for equal. But I guess that makes the sort function harder. I dunno, maybe ignore this one. The list shouldn't be big to really matter.==
, are your post install hooks setup properly?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
scriptsAndStyles
was aMap
to begin with, it wouldn't need to do that relatively expensive step here. And it can still return an array ([...scriptsAndStyles.values()]
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is future-proofing but
scriptsAndStyles
is not used after this point. Could it not just be styles?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
styles
is a fresh array and not used anywhere else. I think this.slice
is redundant.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the
.sort
above not immediately sort this proper?